Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Account for property functions in query rendered features #8665

Merged
merged 3 commits into from
Apr 20, 2017
Merged

Account for property functions in query rendered features #8665

merged 3 commits into from
Apr 20, 2017

Conversation

ivovandongen
Copy link
Contributor

Fixes #8007

@ivovandongen ivovandongen added Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Apr 6, 2017
@ivovandongen ivovandongen self-assigned this Apr 6, 2017
@anandthakker anandthakker self-requested a review April 6, 2017 15:12
@ivovandongen ivovandongen force-pushed the 8007-query-rendered-dds branch 3 times, most recently from 2b82df9 to 46f2d13 Compare April 13, 2017 16:28
@ivovandongen ivovandongen requested a review from kkaefer April 13, 2017 16:29
@ivovandongen
Copy link
Contributor Author

One last failing query test: "line-offset/property-function".

@kkaefer I'm thinking that FeatureIndex::query can use a little cleanup wrt the method arguments. Also, I'm not sure if it makes sense to move Layer::Impl::queryIntersectsFeature over to Bucket or not. Now, the layer gives back exact results, where the bucket uses the statistics to approach it. This might be a good split.

@ivovandongen
Copy link
Contributor Author

ivovandongen commented Apr 14, 2017 •
edited
Loading

I've tracked down the failure in "line-offset/property-function" to a separate bug. The test result is technically correct, but the resulting polygon is translated from:

[
  [
    [-10, 10],
    [10, 10],
    [10, -10],
    [-10, -10],
    [-10, 10]
  ]
]

To

[
  [
    [10, 10],
    [10, -10],
    [-10, -10],
    [-10, 10],
    [10, 10]
  ]
]

Which is functionally the same, but makes the test fail of course. The bug seems to be caused by a workaround in fixupPolygons called when loading the geometries in GeoJSONTileFeature::getGeometries. I'm filing a separate bug for this.

@ivovandongen ivovandongen force-pushed the 8007-query-rendered-dds branch from 46f2d13 to 0c8f1fc Compare April 18, 2017 20:39
@ivovandongen ivovandongen removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 18, 2017
@ivovandongen ivovandongen requested review from jfirebaugh and removed request for anandthakker April 18, 2017 20:40
@ivovandongen
Copy link
Contributor Author

Fixed up the query tests in mapbox/mapbox-gl-js#4603

@ivovandongen ivovandongen force-pushed the 8007-query-rendered-dds branch from 0c8f1fc to 23134a7 Compare April 18, 2017 21:32
jfirebaugh
jfirebaugh suggested changes Apr 18, 2017
View reviewed changes
src/mbgl/geometry/feature_index.hpp Outdated
@@ -1,6 +1,7 @@
#pragma once

#include <mbgl/style/types.hpp>
#include <mbgl/tile/geometry_tile.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer to forward-declare class GeometryTile; rather than #include.

src/mbgl/renderer/bucket.hpp Outdated
@@ -3,8 +3,11 @@
#include <mbgl/renderer/render_pass.hpp>
#include <mbgl/util/noncopyable.hpp>
#include <mbgl/tile/geometry_tile_data.hpp>
#include <mbgl/style/paint_property_statistics.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the added includes leftover from a prior version of this PR? They don't look like they're used.

src/mbgl/style/paint_property_binder.hpp Outdated
@@ -4,6 +4,7 @@
#include <mbgl/gl/attribute.hpp>
#include <mbgl/gl/uniform.hpp>
#include <mbgl/util/type_list.hpp>
#include "paint_property_statistics.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <mbgl/...>

src/mbgl/style/paint_property_binder.hpp Outdated
@@ -76,24 +77,24 @@ std::array<float, N*2> zoomInterpolatedAttributeValue(const std::array<float, N>
`glVertexAttrib*` was unnacceptably slow. Additionally, in GL Native we have
implemented binary shader caching, which works better if the shaders are constant.
*/
template <class T, class A>
template <class P, class T, class A, class Statistics>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specializing on unique P and Statistics increases the binary size significantly:

~/Development/mapbox-gl-native $ ../bloaty/bloaty -d symbols build/macos/Release/Mapbox.framework/Versions/Current/Mapbox -- build-before/macos/Release/Mapbox.framework/Versions/Current/Mapbox 
     VM SIZE                                                                                        FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
   +95%  +111Ki [Other]                                                                           +111Ki   +95%
  +2.0% +9.62Ki [None]                                                                           +8.23Ki  +1.7%
  [NEW] +1.06Ki mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s +1.06Ki  [NEW]
  [NEW]    +816 mbgl::style::LineLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates c    +816  [NEW]
  [NEW]    +632 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::FillOutlineColor,    +632  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::CircleColor, mbgl    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::IconHaloColor, mb    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::TextHaloColor, mb    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::CircleStrokeColor    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::FillColor, mbgl::    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::IconColor, mbgl::    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::LineColor, mbgl::    +620  [NEW]
  [NEW]    +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::TextColor, mbgl::    +620  [NEW]
  [NEW]    +464 GCC_except_table681                                                                 +464  [NEW]
  [NEW]    +434 mbgl::LineBucket::getQueryRadius(mbgl::style::Layer const&) const                   +434  [NEW]
   +46%    +432 GCC_except_table37                                                                  +432   +46%
  [NEW]    +426 mbgl::style::CircleLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates    +426  [NEW]
  [NEW]    +384 GCC_except_table717                                                                 +384  [NEW]
  [NEW]    +352 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::CircleBlur, float    +352  [NEW]
  [NEW]    +352 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::LineOffset, float    +352  [NEW]
  [NEW]    +352 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::FillOpacity, floa    +352  [NEW]

 -------------- SHRINKING                                                                        --------------
 -12.8% -13.2Ki [Other]                                                                          -13.2Ki -12.8%
  [DEL]    -642 mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s    -642  [DEL]
  [DEL]    -630 mbgl::style::LineLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates     -630  [DEL]
  [DEL]    -624 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribu    -624  [DEL]
 -65.3%    -384 GCC_except_table718                                                                 -384 -65.3%
  [DEL]    -358 mbgl::style::CircleLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinate    -358  [DEL]
  [DEL]    -336 mbgl::style::CompositeFunctionPaintPropertyBinder<float, mbgl::gl::Attribute<flo    -336  [DEL]
  [DEL]    -302 mbgl::style::FillLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates     -302  [DEL]
  [DEL]    -292 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribu    -292  [DEL]
  [DEL]    -286 mbgl::style::PaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute<float, 2ul> >:    -286  [DEL]
 -86.4%    -280 GCC_except_table401                                                                 -280 -86.4%
  [DEL]    -280 mbgl::style::CompositeFunctionPaintPropertyBinder<float, mbgl::gl::Attribute<flo    -280  [DEL]
 -52.4%    -260 GCC_except_table682                                                                 -260 -52.4%
  [DEL]    -260 mbgl::style::PaintPropertyBinder<float, mbgl::gl::Attribute<float, 1ul> >::creat    -260  [DEL]
  [DEL]    -248 mbgl::style::PaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute<float, 2ul> >:    -248  [DEL]
 -40.1%    -236 GCC_except_table180                                                                 -236 -40.1%
  [DEL]    -228 mbgl::style::PaintPropertyBinder<float, mbgl::gl::Attribute<float, 1ul> >::creat    -228  [DEL]
  [DEL]    -224 mbgl::style::SourceFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute<    -224  [DEL]
 -50.5%    -220 GCC_except_table491                                                                 -220 -50.5%
  [DEL]    -204 GCC_except_table683                                                                 -204  [DEL]
  [DEL]    -200 mbgl::style::SourceFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute<    -200  [DEL]

  +3.3%  +112Ki TOTAL                                                                             +110Ki  +3.2%

Can we find a way to specialize only on the underlying type (e.g. float)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the approach I suggest: 2b83d39.

     VM SIZE                                                                                        FILE SIZE
 ++++++++++++++ GROWING                                                                          ++++++++++++++
  +9.0% +8.72Ki [Other]                                                                          +8.72Ki  +9.0%
  +0.2% +1.21Ki [None]                                                                           +1.28Ki  +0.3%
  [NEW] +1.06Ki mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s +1.06Ki  [NEW]
  [NEW]    +816 mbgl::style::LineLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates c    +816  [NEW]
  [NEW]    +464 GCC_except_table681                                                                 +464  [NEW]
   +46%    +432 GCC_except_table37                                                                  +432   +46%
  [NEW]    +432 mbgl::LineBucket::getQueryRadius(mbgl::style::Layer const&) const                   +432  [NEW]
  [NEW]    +426 mbgl::style::CircleLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates    +426  [NEW]
  [NEW]    +384 GCC_except_table717                                                                 +384  [NEW]
  [NEW]    +348 mbgl::style::FillLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates c    +348  [NEW]
  +116%    +296 GCC_except_table179                                                                 +296  +116%
  +206%    +280 GCC_except_table400                                                                 +280  +206%
  +182%    +248 GCC_except_table231                                                                 +248  +182%
  +219%    +228 GCC_except_table358                                                                 +228  +219%
  [NEW]    +204 GCC_except_table412                                                                 +204  [NEW]
  [NEW]    +184 std::__1::vector<mbgl::style::Layer const*, std::__1::allocator<mbgl::style::Lay    +184  [NEW]
  [NEW]    +178 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch    +178  [NEW]
  [NEW]    +175 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch    +175  [NEW]
  [NEW]    +164 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch    +164  [NEW]
  [NEW]    +164 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch    +164  [NEW]
  [NEW]    +162 mbgl::style::Style::getLayers() const                                               +162  [NEW]

 -------------- SHRINKING                                                                        --------------
  -8.9% -7.34Ki [Other]                                                                          -7.34Ki  -8.9%
  [DEL]    -642 mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s    -642  [DEL]
  [DEL]    -630 mbgl::style::LineLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates     -630  [DEL]
 -65.3%    -384 GCC_except_table718                                                                 -384 -65.3%
  [DEL]    -358 mbgl::style::CircleLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinate    -358  [DEL]
  [DEL]    -302 mbgl::style::FillLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates     -302  [DEL]
 -86.4%    -280 GCC_except_table401                                                                 -280 -86.4%
 -64.2%    -280 GCC_except_table491                                                                 -280 -64.2%
 -52.4%    -260 GCC_except_table682                                                                 -260 -52.4%
 -40.1%    -236 GCC_except_table180                                                                 -236 -40.1%
  [DEL]    -204 GCC_except_table683                                                                 -204  [DEL]
 -80.7%    -184 GCC_except_table359                                                                 -184 -80.7%
 -16.8%    -184 GCC_except_table43                                                                  -184 -16.8%
 -13.9%    -180 GCC_except_table38                                                                  -180 -13.9%
  -4.0%    -180 GCC_except_table6                                                                   -180  -4.0%
 -14.2%    -172 GCC_except_table41                                                                  -172 -14.2%
  [DEL]    -162 mbgl::style::LineLayer::Impl::getQueryRadius() const                                -162  [DEL]
 -77.6%    -152 GCC_except_table435                                                                 -152 -77.6%
  [DEL]    -152 GCC_except_table723                                                                 -152  [DEL]
 -54.5%    -144 GCC_except_table413                                                                 -144 -54.5%
  [DEL]    -140 GCC_except_table499                                                                 -140  [DEL]

  +0.1% +4.00Ki TOTAL                                                                            +4.06Ki  +0.1%

@ivovandongen ivovandongen force-pushed the 8007-query-rendered-dds branch 2 times, most recently from 3833732 to 83d4618 Compare April 19, 2017 17:46
@ivovandongen
Copy link
Contributor Author

Thanks @jfirebaugh I've worked in the changes. Only blocked until #8760 is merged as the render tests are failing.

jfirebaugh
jfirebaugh approved these changes Apr 19, 2017
View reviewed changes
src/mbgl/style/paint_property_binder.hpp Outdated
@@ -136,6 +139,8 @@ class SourceFunctionPaintPropertyBinder : public PaintPropertyBinder<T, A> {
}

void populateVertexVector(const GeometryTileFeature& feature, std::size_t length) override {
auto evaluated = function.evaluate(feature, defaultValue);
this->statistics.add(evaluated);
auto value = attributeValue(function.evaluate(feature, defaultValue));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally reverted this line in my changes, should be auto value = attributeValue(evaluated);

@ivovandongen ivovandongen force-pushed the 8007-query-rendered-dds branch 3 times, most recently from 87d4e57 to 33c7707 Compare April 19, 2017 21:29
@ivovandongen ivovandongen force-pushed the 8007-query-rendered-dds branch from 33c7707 to 59a92db Compare April 19, 2017 23:24
@ivovandongen ivovandongen merged commit 7ddca3b into master Apr 20, 2017
@ivovandongen ivovandongen deleted the 8007-query-rendered-dds branch April 20, 2017 00:21
@1ec5 1ec5 mentioned this pull request May 1, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jfirebaugh jfirebaugh jfirebaugh approved these changes

@kkaefer kkaefer Awaiting requested review from kkaefer

Assignees

@ivovandongen ivovandongen

Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivovandongen @jfirebaugh